Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG] Fixes lacking mutex protection to some ACK-related fields #2723

Merged
merged 1 commit into from
Sep 19, 2023
Merged

[BUG] Fixes lacking mutex protection to some ACK-related fields #2723

merged 1 commit into from
Sep 19, 2023

Conversation

ethouris
Copy link
Collaborator

Fixes #2706

This was initially created to verify #2706 problem causing a hangup with high bitrates on the sending side.

The suspected part was lack of mutex protection for the activities around ACK reception and the sequence number pointers m_iSndLastAck and m_iSndCurrSeqNo which could be modified simultaneously by the receiver thread upon reception of UMSG_ACK. There was a distinct corner case with possible modification of one of these fields during processing, which could cause a situation of impossible layout of the sequence numbers likely after sender-dropping packets.

Note: it is not clear what exactly has fixed the problem, but with this version the reported problem was no longer reproducible.

@ethouris ethouris added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Apr 20, 2023
@maxsharabayko maxsharabayko modified the milestones: v1.5.2, v1.6.0 Apr 20, 2023
@DevSysEngineer
Copy link

I hope this solution will be included in version 1.5.2. I have plans to send high bitrate streams (4K UHD) over the internet soon. What is the reason for not including this fix? Rather wait an extra week for testing, but then you have a version that is super stable for coming months.

@ethouris ethouris mentioned this pull request Apr 25, 2023
Open
6 tasks
@maxsharabayko
Copy link
Collaborator

I hope this solution will be included in version 1.5.2. I have plans to send high bitrate streams (4K UHD) over the internet soon. What is the reason for not including this fix? Rather wait an extra week for testing, but then you have a version that is super stable for coming months.

Hi @KvanSteijn,
This PR touches mutexes, which may introduce new issues like deadlocks. If the PR is taken to v1.5.2, we would need to repeat the whole QA testing. Those are already allocated to other tasks. Given the issue is not a regression, there is no reason to include it in v1.5.2. Although I understand your motivation as well.

I would also like to bring your attention to the following note:

Note: it is not clear what exactly has fixed the problem, but with this version the reported problem was no longer reproducible.

The fix was not properly confirmed, logs and/or pcaps are still expected. Please see this comment.

@ethouris
Copy link
Collaborator Author

Note: the proper testing for any deadlocks possibility will be possible after merging #2743.

@maxsharabayko maxsharabayko merged commit 6c723e5 into Haivision:master Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxsharabayko maxsharabayko Awaiting requested review from maxsharabayko

Assignees
No one assigned
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Milestone
v1.5.4
Development

Successfully merging this pull request may close these issues.

[BUG] srt_send() hangs forever with high bitrates
3 participants
@ethouris @DevSysEngineer @maxsharabayko